Allow gatewayProvisioner to create contour that only watch limited na… #6073

lubronzhan · 2024-01-12T00:17:27Z

…mespaces of resources

Based on change of @padlar here

Previous PR didn't take care of the case of GatewayProvisioner. Since gatewayProvisioner has its own manager, and gatewayClass is a clusterscope variable

Now, if customer set spec.contour.watchNamespaces in ContourDeployment, the contour instance created by gatewayProvisioner will only watch namespaces under spec.contour.watchNamespaces and the resources under the namespace where the contour instance is located.

I have one concern is, if customer doesn't include ContourDeployment's namespace in watchNamespace, do we automatically add it to the Contour's watchNamespace, or we put condition inside Gateway to show that this is an issue, that they should include the namespace in the watchNamespace. Open to ideas about whether to choose 1st or 2nd option

lubronzhan · 2024-01-12T00:21:43Z

How do I set the label of release-note?

codecov · 2024-01-12T00:25:33Z

Codecov Report

Attention: 71 lines in your changes are missing coverage. Please review.

Comparison is base (9eb2838) 78.82% compared to head (0746ba4) 78.52%.
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6073      +/-   ##
==========================================
- Coverage   78.82%   78.52%   -0.31%     
==========================================
  Files         138      140       +2     
  Lines       19766    19911     +145     
==========================================
+ Hits        15581    15635      +54     
- Misses       3878     3967      +89     
- Partials      307      309       +2

Files	Coverage Δ
internal/provisioner/controller/gateway.go	`62.27% <100.00%> (+0.22%)`	⬆️
internal/provisioner/model/model.go	`100.00% <100.00%> (ø)`
internal/provisioner/model/names.go	`100.00% <100.00%> (ø)`
...ernal/provisioner/objects/deployment/deployment.go	`89.25% <100.00%> (+0.27%)`	⬆️
...ovisioner/objects/rbac/clusterrole/cluster_role.go	`64.70% <100.00%> (-15.63%)`	⬇️
internal/provisioner/objects/rbac/util/util.go	`100.00% <100.00%> (ø)`
...ovisioner/objects/rbac/rolebinding/role_binding.go	`56.36% <40.00%> (-15.07%)`	⬇️
internal/provisioner/objects/rbac/role/role.go	`62.31% <41.93%> (-7.45%)`	⬇️
internal/provisioner/objects/rbac/rbac.go	`12.00% <6.81%> (-1.64%)`	⬇️

... and 3 files with indirect coverage changes

skriss · 2024-01-12T16:54:04Z

@lubronzhan the other thing we want to do here is generate per-namespace Roles/RoleBindings for Contour, instead of the default ClusterRole/ClusterRoleBinding, since the user typically wants/should want least privileges when restricting to 1+ specific namespace(s). Effectively doing the same as what's in https://github.com/projectcontour/contour/blob/main/examples/namespaced/kustomization.yaml. The code for the provisioner generating RBAC resources is in https://github.com/projectcontour/contour/tree/main/internal/provisioner/objects/rbac

skriss

We'll definitely want an E2E test on this, to ensure we got all the RBAC right. See https://github.com/projectcontour/contour/blob/main/test/e2e/provisioner/provisioner_test.go for existing provisioner tests that instantiate a Contour and test traffic routing.

apis/projectcontour/v1alpha1/contourdeployment.go

internal/provisioner/model/model.go

lubronzhan · 2024-01-12T23:25:34Z

Effectively doing the same as what's in main/examples/namespaced/kustomization.yaml. The code for the provisioner generating RBAC resources is in main/internal/provisioner/objects/rbac

Ohk, I thought just doing the kustomization is enough, for example I added an example here. https://github.com/lubronzhan/contour/blob/topic/lubron/fix-5256/examples/namespaced-gatewayapi/kustomization.yaml
ohk let me change that https://github.com/projectcontour/contour/tree/main/internal/provisioner/objects/rbac?rgh-link-date=2024-01-12T16%3A54%3A04Z

Do we have e2e test for the kustomization as well? or we just manually test it

internal/provisioner/objects/rbac/rbac.go

lubronzhan · 2024-01-17T05:48:53Z

Ok the code-gene difference is

-                    description: WatchNamespaces is an array of namespaces. Setting
-                      it will instruct the contour instance to only watch these set
-                      of namespaces
+                    description: |-
+                      WatchNamespaces is an array of namespaces. Setting it will instruct the contour instance
+                      to only watch these set of namespaces

Turns out I need to rebase

Signed-off-by: lubronzhan <lubronzhan@gmail.com> Signed-off-by: Lubron Zhan <lubronzhan@gmail.com>

Signed-off-by: Lubron Zhan <lubronzhan@gmail.com>

apis/projectcontour/v1alpha1/contourdeployment.go

internal/provisioner/model/model.go

Signed-off-by: Lubron Zhan <lubronzhan@gmail.com>

sunjayBhatia

I think this is pretty much good to go minus some small things, one thing I would maybe change too is to put the new API Namespace type in the v1 package so we can use it in that package later as well, better to have a type there and use it in v1alpha than the other way round I think (and not have to define it twice)

apis/projectcontour/v1alpha1/contourdeployment_helpers_test.go

test/e2e/provisioner/provisioner_test.go

test/e2e/framework.go

internal/provisioner/objects/deployment/deployment_test.go

Signed-off-by: Lubron Zhan <lubronzhan@gmail.com>

skriss

Couple more tiny spelling nits, otherwise I think this is about good to go as well

internal/provisioner/model/names.go

test/e2e/gatewayapi_predicates.go

Signed-off-by: Lubron Zhan <lubronzhan@gmail.com>

lubronzhan · 2024-01-29T20:06:11Z

Thanks!

internal/provisioner/objects/rbac/util/util.go

Signed-off-by: lubronzhan <lubronzhan@gmail.com>

izturn · 2024-01-30T09:06:28Z

@lubronzhan LGTM thx

lubronzhan requested a review from a team as a code owner January 12, 2024 00:17

lubronzhan requested review from skriss and sunjayBhatia and removed request for a team January 12, 2024 00:17

sunjayBhatia requested review from a team and clayton-gonsalves and removed request for a team January 12, 2024 00:17

lubronzhan force-pushed the topic/lubron/fix-5256 branch 2 times, most recently from a237ddf to 45b732e Compare January 12, 2024 00:20

lubronzhan changed the title ~~Allow gatewayProvisioner to create contour that only watch limited na…~~ [WIP]Allow gatewayProvisioner to create contour that only watch limited na… Jan 12, 2024

lubronzhan force-pushed the topic/lubron/fix-5256 branch 2 times, most recently from d205e85 to a52c282 Compare January 12, 2024 06:05

skriss requested a review from izturn January 12, 2024 16:40

skriss requested changes Jan 12, 2024

View reviewed changes

apis/projectcontour/v1alpha1/contourdeployment.go Outdated Show resolved Hide resolved

internal/provisioner/model/model.go Outdated Show resolved Hide resolved

lubronzhan force-pushed the topic/lubron/fix-5256 branch from 0860e61 to 16d413d Compare January 14, 2024 07:55

izturn added the area/gateway-provisioner Issues or PRs related to the Gateway provisioner label Jan 15, 2024

izturn assigned lubronzhan Jan 15, 2024

izturn added the release-note/small A small change that needs one line of explanation in the release notes. label Jan 15, 2024

izturn reviewed Jan 15, 2024

View reviewed changes

internal/provisioner/objects/rbac/rbac.go Outdated Show resolved Hide resolved

lubronzhan force-pushed the topic/lubron/fix-5256 branch from 50fe842 to 9b95033 Compare January 17, 2024 04:50

This was referenced Jan 17, 2024

golangci-lint is not showing line number for error #6095

Closed

Add flag to golangci-lint to show line number if there is error #6096

Merged

lubronzhan force-pushed the topic/lubron/fix-5256 branch 2 times, most recently from 34281ac to 189131c Compare January 17, 2024 05:48

lubronzhan force-pushed the topic/lubron/fix-5256 branch from c2b8cb2 to 5ae1c67 Compare January 17, 2024 06:09

lubronzhan added 3 commits January 23, 2024 16:42

Initial code of e2e test

80c6735

Signed-off-by: lubronzhan <lubronzhan@gmail.com> Signed-off-by: Lubron Zhan <lubronzhan@gmail.com>

Validate e2e tests locally

e729fca

Signed-off-by: Lubron Zhan <lubronzhan@gmail.com>

Resolve comments

3fb1e9c

Signed-off-by: Lubron Zhan <lubronzhan@gmail.com>

lubronzhan force-pushed the topic/lubron/fix-5256 branch from df62c3e to 3fb1e9c Compare January 24, 2024 00:43

lubronzhan mentioned this pull request Jan 24, 2024

Change behavior of watchNamespaces on contourDeployment to not include gateway's namespace by default #6120

Open

sunjayBhatia reviewed Jan 24, 2024

View reviewed changes

apis/projectcontour/v1alpha1/contourdeployment.go Outdated Show resolved Hide resolved

sunjayBhatia reviewed Jan 24, 2024

View reviewed changes

internal/provisioner/model/model.go Outdated Show resolved Hide resolved

Basic validation

4455855

Signed-off-by: Lubron Zhan <lubronzhan@gmail.com>

lubronzhan force-pushed the topic/lubron/fix-5256 branch 2 times, most recently from 5746d35 to d7a63b1 Compare January 26, 2024 01:02

Add namespace alias for string for validation purpose

c8666ad

Signed-off-by: Lubron Zhan <lubronzhan@gmail.com>

lubronzhan force-pushed the topic/lubron/fix-5256 branch from d7a63b1 to c8666ad Compare January 26, 2024 01:34

sunjayBhatia requested review from skriss, sunjayBhatia and izturn January 26, 2024 19:12

sunjayBhatia reviewed Jan 26, 2024

View reviewed changes

lubronzhan added 2 commits January 26, 2024 15:34

Resolve comments

7fc8fed

Signed-off-by: Lubron Zhan <lubronzhan@gmail.com>

Remove kustimization

ca7c2cd

Signed-off-by: Lubron Zhan <lubronzhan@gmail.com>

lubronzhan force-pushed the topic/lubron/fix-5256 branch from f1fd602 to ca7c2cd Compare January 26, 2024 23:35

Make generate

c179a3c

Signed-off-by: Lubron Zhan <lubronzhan@gmail.com>

skriss requested changes Jan 29, 2024

View reviewed changes

internal/provisioner/model/names.go Outdated Show resolved Hide resolved

internal/provisioner/model/names.go Outdated Show resolved Hide resolved

test/e2e/gatewayapi_predicates.go Outdated Show resolved Hide resolved

More comments

c2d5d1d

Signed-off-by: Lubron Zhan <lubronzhan@gmail.com>

izturn reviewed Jan 30, 2024

View reviewed changes

internal/provisioner/objects/rbac/util/util.go Outdated Show resolved Hide resolved

Resolve comments

0746ba4

Signed-off-by: lubronzhan <lubronzhan@gmail.com>

skriss approved these changes Jan 30, 2024

View reviewed changes

sunjayBhatia approved these changes Jan 30, 2024

View reviewed changes

skriss merged commit 29201bb into projectcontour:main Jan 30, 2024
25 of 26 checks passed

lubronzhan deleted the topic/lubron/fix-5256 branch January 30, 2024 23:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow gatewayProvisioner to create contour that only watch limited na… #6073

Allow gatewayProvisioner to create contour that only watch limited na… #6073

lubronzhan commented Jan 12, 2024 •

edited

Loading

lubronzhan commented Jan 12, 2024

codecov bot commented Jan 12, 2024 •

edited

Loading

skriss commented Jan 12, 2024

skriss left a comment

lubronzhan commented Jan 12, 2024

lubronzhan commented Jan 17, 2024 •

edited

Loading

sunjayBhatia left a comment

skriss left a comment

lubronzhan commented Jan 29, 2024

izturn commented Jan 30, 2024

Allow gatewayProvisioner to create contour that only watch limited na… #6073

Allow gatewayProvisioner to create contour that only watch limited na… #6073

Conversation

lubronzhan commented Jan 12, 2024 • edited Loading

lubronzhan commented Jan 12, 2024

codecov bot commented Jan 12, 2024 • edited Loading

Codecov Report

skriss commented Jan 12, 2024

skriss left a comment

Choose a reason for hiding this comment

lubronzhan commented Jan 12, 2024

lubronzhan commented Jan 17, 2024 • edited Loading

sunjayBhatia left a comment

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

lubronzhan commented Jan 29, 2024

izturn commented Jan 30, 2024

lubronzhan commented Jan 12, 2024 •

edited

Loading

codecov bot commented Jan 12, 2024 •

edited

Loading

lubronzhan commented Jan 17, 2024 •

edited

Loading